grt: getCost refactor and mazeRouteMSMD relax grids refactor #5906
Conversation
Signed-off-by: João Mai <[email protected]>
Signed-off-by: João Mai <[email protected]>
Signed-off-by: João Mai <[email protected]>
Signed-off-by: João Mai <[email protected]>
…o fastroute-rafactor-mazeRouteMSMD
Signed-off-by: João Mai <[email protected]>
Signed-off-by: João Mai <[email protected]>
…o fastroute-rafactor-mazeRouteMSMD
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Signed-off-by: João Mai <[email protected]>
Signed-off-by: João Mai <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: João Mai <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
| : logis_cof(logis_cof), | ||
| cost_height(cost_height), | ||
| slope(slope), | ||
| cost_type(cost_type) |
There was a problem hiding this comment.
It would be nice to understand what each cost type means and change it from int to an enum class, with descritive names. Today we only compare with numbers, which is very unclear.
| const float cost_height, | ||
| const int slope, | ||
| const int cost_type) | ||
| : logis_cof(logis_cof), |
There was a problem hiding this comment.
I think logis_cof could be renamed to something clearer.
src/grt/src/fastroute/src/maze.cpp
Outdated
| const int slope, | ||
| const int capacity, | ||
| const int cost_type) | ||
| double FastRouteCore::getCost(const int i, |
There was a problem hiding this comment.
The parameter i could have a more descriptive name. Looking at the usage of this functions, the code is passing a pos, but it doesn't seem to be a position relative to the grid.
There was a problem hiding this comment.
It is probably a position related to the heap.
…o fastroute-refactor-mazeRouteMSMD
Signed-off-by: João Mai <[email protected]>
Signed-off-by: João Mai <[email protected]>
Signed-off-by: João Mai <[email protected]>
Signed-off-by: João Mai <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@joaomai The changes look good, but you will need to update the metrics for the failing designs in the secure-ci. Only then we can merge this PR. |
…o fastroute-refactor-mazeRouteMSMD
|
clang-tidy review says "All clean, LGTM! 👍" |
…o fastroute-refactor-mazeRouteMSMD
Signed-off-by: João Mai <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
conflicts to resolve |
…o fastroute-refactor-mazeRouteMSMD
|
clang-tidy review says "All clean, LGTM! 👍" |
getCost function refactoring to remove the need of checking if the cost was already calculated for a given position.
relaxAdjacent and updateAdjacent were made as lambdas since both need to access and modify local variables to the mazeRouteMSMD function.
When relaxing the left grid the cost was stored using float, when the other directions were using double. The change to double caused changes in metrics such as total usage and via count for some tests. Similarly, both cost tables were using double while getCost returned a float, which was updated to use double too.